Skip to content

Record the command's id in CFSetExitCode for built-in handlers - #3516

Open
Eljees wants to merge 1 commit into
koalaman:masterfrom
Eljees:fix/3490-exit-code-id-for-builtins
Open

Record the command's id in CFSetExitCode for built-in handlers#3516
Eljees wants to merge 1 commit into
koalaman:masterfrom
Eljees:fix/3490-exit-code-id-for-builtins

Conversation

@Eljees

@Eljees Eljees commented Aug 9, 2026

Copy link
Copy Markdown

Fixes #3490.

Symptom

SC2320 fires for echo but not for printf:

#!/bin/sh
mycommand
printf 'Command exited with %d\n' $?
if [ $? -ne 0 ]      # no SC2320 here
then
  echo "Failed"
fi

A four-way probe puts the discriminator on the command name rather than on quoting:

script before after
printf '...%d' $? silent SC2320
printf '...%d' "$?" silent SC2320
echo "...$?" SC2320 SC2320
echo $? SC2320 SC2320

Cause

handleCommand cmd vars args literalCmd receives the whole T_SimpleCommand as cmd, and the ordinary path registers the exit code against it:

regular = handleOthers (getId cmd) vars args literalCmd

regularExpansionWithStatus, which the built-in table uses, shadows that name with the command's first word:

regularExpansionWithStatus vars args@(cmd NE.:| _) p = do
    initial <- regularExpansion vars (NE.toList args) p
    status  <- newNodeRange $ CFSetExitCode (getId cmd)   -- id of the word, not the command

So printf, unset, wait, mapfile, readarray, read and the four DEFINE_* commands all register their exit code under the id of a T_NormalWord. A consumer that resolves that id through idMap then gets a word where it expects a command, and checkOverwrittenExitCode is exactly such a consumer: getCommandBasename is Nothing for a T_NormalWord, so isPrinting never matches. echo is not in the table, goes through handleOthers, and therefore works.

Fix

Drop the shadowing pattern so cmd refers to the command again, matching handleOthers. One token; the body of the helper is unchanged.

Tests

Two prop_ cases next to the existing ones:

prop_checkOverwrittenExitCode9  = verify    checkOverwrittenExitCode "x; printf '%d' $?; [ $? -eq 0 ]"
prop_checkOverwrittenExitCode10 = verifyNot checkOverwrittenExitCode "read -r x; [ $? -eq 0 ]"

The second pins the intended narrowness: read also goes through the helper and also gets a corrected id, but it is neither a condition nor a printing command, so nothing new is reported for it.

Verified on GHC 9.8.4:

  • with the tests but without the fix, cabal test fails (*** Failed! Falsified (after 1 test), Test suite test-shellcheck: FAIL);
  • with the fix, cabal test passes in full;
  • end-to-end per CLAUDE.md: the reporter's script now reports SC2320, echo is unchanged, and mycommand; unset foo; [ $? -eq 0 ] stays silent apart from the unrelated SC2181.

AI usage

I used Claude to help trace the CFG path and to draft the patch and the two tests, following the workflow in this repository's .claude/CLAUDE.md. I read every line of the change, ran the four-way probe to establish the cause rather than assume it, ran the new tests against unpatched master first to confirm they fail without the fix, and ran the full cabal test and the end-to-end checks myself.

handleCommand receives the whole T_SimpleCommand as `cmd`, and the ordinary
path registers the exit code against it through
`handleOthers (getId cmd) ...`. regularExpansionWithStatus, used by the
built-in table, shadowed that name with the command's first word, so printf,
unset, wait, mapfile, readarray, read and the four DEFINE_* commands recorded
their exit code under the id of a T_NormalWord instead.

Consumers that resolve the id through idMap then get a word where they expect
a command. checkOverwrittenExitCode is one of them: getCommandBasename is
Nothing for a T_NormalWord, so isPrinting never matched and SC2320 stayed
silent for printf while firing for echo, which is not in the table and goes
through handleOthers.

Drop the shadowing pattern so cmd again refers to the command.

Fixes koalaman#3490
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SC2320: false negative for printf

1 participant